Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add Parallel.parFoldMapA #3147

Merged
merged 3 commits into from
Nov 14, 2019
Merged

Add Parallel.parFoldMapA #3147

merged 3 commits into from
Nov 14, 2019

Conversation

nigredo-tori
Copy link
Contributor

Aka foldMapA hoisted through a Parallel.

@LukaJCB
Copy link
Member

LukaJCB commented Nov 13, 2019

This is a great feature!

I wonder though if it would be easier to just place this method straight on the Foldable trait?

@nigredo-tori
Copy link
Contributor Author

nigredo-tori commented Nov 13, 2019 •
edited
Loading

It would probably be easier (no extra syntax). I'm a bit out of the loop, though - is this the preferred way to do things at the moment? I put parFoldMapA on Parallel to keep it consistent with all the other par* functions, but I can put it on Foldable if you want.

@codecov-io
Copy link

codecov-io commented Nov 13, 2019 •
edited
Loading

Codecov Report

Merging #3147 into master will decrease coverage by 0.02%.
The diff coverage is 50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3147      +/-   ##
==========================================
- Coverage   93.28%   93.25%   -0.03%     
==========================================
  Files         376      376              
  Lines        7323     7327       +4     
  Branches      195      199       +4     
==========================================
+ Hits         6831     6833       +2     
- Misses        492      494       +2
Flag Coverage Δ
#scala_version_212 93.59% <50%> (-0.03%) ⬇️
#scala_version_213 90.92% <50%> (-0.03%) ⬇️
Impacted Files Coverage Δ
core/src/main/scala/cats/syntax/parallel.scala 58.82% <0%> (-3.68%) ⬇️
core/src/main/scala/cats/Parallel.scala 84.84% <100%> (+0.47%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9bb9636...0259244. Read the comment docs.

travisbrown
travisbrown suggested changes Nov 13, 2019
View reviewed changes
Copy link
Contributor

@travisbrown travisbrown left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great, thanks @nigredo-tori!

I think it probably is best to keep this on Parallel for consistency. I've made a few comments, but not adding another BinCompatN trait is the only important one.

Would you mind adding a syntax test to SyntaxSuite as well?

core/src/main/scala/cats/Parallel.scala Outdated Show resolved Hide resolved
core/src/main/scala/cats/syntax/all.scala Outdated Show resolved Hide resolved
core/src/main/scala/cats/Parallel.scala Outdated Show resolved Hide resolved
core/src/main/scala/cats/syntax/parallel.scala Show resolved Hide resolved
@travisbrown travisbrown added this to the 2.1.0-RC1 milestone Nov 13, 2019
travisbrown
travisbrown approved these changes Nov 14, 2019
View reviewed changes
Copy link
Contributor

@travisbrown travisbrown left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, thanks!

rossabaker
rossabaker approved these changes Nov 14, 2019
View reviewed changes
Copy link
Member

@rossabaker rossabaker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that it's in the right place where it is. Thanks!

@travisbrown travisbrown merged commit bd82ff2 into typelevel:master Nov 14, 2019
@nigredo-tori nigredo-tori deleted the parfoldmapa branch November 15, 2019 01:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@travisbrown travisbrown travisbrown approved these changes

@rossabaker rossabaker rossabaker approved these changes

@LukaJCB LukaJCB Awaiting requested review from LukaJCB

Assignees
No one assigned
Labels
enhancement
Projects
None yet
Milestone
2.1.0-RC1
Development

Successfully merging this pull request may close these issues.
5 participants
@nigredo-tori @LukaJCB @codecov-io @rossabaker @travisbrown